Export findings from the backend instead of loaded page state - #2453
Open
Subramaniyajothi6 wants to merge 2 commits into
Open
Export findings from the backend instead of loaded page state#2453Subramaniyajothi6 wants to merge 2 commits into
Subramaniyajothi6 wants to merge 2 commits into
Conversation
utksh1
requested changes
Aug 4, 2026
utksh1
left a comment
Owner
There was a problem hiding this comment.
This is a substantial and security-sensitive export change, but it is currently merge-conflicted (mergeable=false, mergeState=dirty) and the required frontend checks are failing. Please rebase onto current main, resolve the Findings UI conflicts, and push a green frontend run before merge. After that I will re-review the owner scoping, redaction, pagination/batching, and SARIF contracts.
Subramaniyajothi6
force-pushed
the
feature/bulk-export-findings-1875
branch
from
August 4, 2026 11:15
bf74a7e to
1678ed8
Compare
Findings export was built entirely in the browser from React state, so it could only ever contain findings already scrolled into memory. Selecting across pages exported whatever happened to be loaded. Add POST /api/v1/findings/export, which resolves a selection of finding ids against the database and streams CSV, JSON, or SARIF back. Omitting the ids exports everything the caller owns; an empty array exports nothing and is never read as "everything". Findings are read in batches and serialized as they go, so memory is bounded by SECUSCAN_EXPORT_BATCH_SIZE rather than by the size of the export. SARIF is the exception and is assembled in full, because its deduplicated rules array precedes the results that index into it. SECUSCAN_MAX_EXPORT_FINDINGS caps a request rather than truncating it, so an export is never silently partial. Every query is owner-scoped. Ids belonging to another owner are skipped rather than rejected, so the endpoint cannot be used to probe for them, and the reported count does not confirm them either. Free-text fields, evidence, and metadata go through the same redaction as task reports. The CSV keeps the columns the browser produced, so existing scripts still work. The client-side serializers they came from are removed; their column contract now lives in testing/backend/unit/test_finding_export.py.
Moving CSV generation to the backend reintroduced CWE-1236: cells were
written raw, so a finding titled =HYPERLINK("http://attacker","click")
became a live formula when the export was opened in a spreadsheet.
Finding titles, targets and descriptions carry scanner output — page
titles, banners, reflected headers — so the content is attacker-influenced.
Prefix any cell starting with =, +, - or @ with a single quote, which makes
the spreadsheet read it as literal text. Applied to every column rather than
the free-text ones: a column that is only safe while the data is well-formed
is not a guarantee worth relying on.
This deliberately mirrors ReportGenerator._sanitize_csv_cell, which utksh1#2394
adds for the task-report CSV. Both write findings into a spreadsheet and
must not disagree about what is safe. They should be folded into one helper
once both have landed; they are separate only because utksh1#2394 is unmerged and
this path has to be safe on its own.
Subramaniyajothi6
force-pushed
the
feature/bulk-export-findings-1875
branch
from
August 5, 2026 11:54
1678ed8 to
bb99007
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1875
Problem
Findings export was assembled entirely in the browser:
findingsis React state, so only findings already scrolled into memory could ever end up in the file. Select across pages and you got whatever happened to be loaded — silently, with no indication anything was missing.Changes
POST /api/v1/findings/exportresolves a selection of finding ids against the database and streamscsv,json, orsarifback.Three selection cases, deliberately distinct:
finding_idsnull["a", "b"][]That last row matters: an empty selection must never be read as "everything". It is pinned by a test.
Memory
Findings are read in batches of
SECUSCAN_EXPORT_BATCH_SIZE(500) and serialized as they go, so memory tracks the batch size rather than the size of the export. Both the read and the id-count are batched — anINlist is one bound parameter per id, and every database has a ceiling on those.SARIF is the exception and is assembled in full. Its deduplicated
rulesarray lives in the tool driver ahead of the results that index into it, so the whole set has to be known before the first byte is correct.SECUSCAN_MAX_EXPORT_FINDINGS(10000) is what bounds that; it rejects an oversized request rather than truncating it, so an export is never quietly partial.Security
owner_id. Ids belonging to someone else are skipped rather than rejected, so the endpoint cannot be used to test whether a finding exists — andX-Export-Finding-Countdoes not confirm them either. Both are tested.target,description,remediation,proof,confidence_reason, evidence, and metadata go through the sameredaction.pyhelpers task reports use. One function gates every byte leaving the module, so CSV, JSON, and SARIF cannot drift apart.report_download_limiter, shared with report downloads.owner_idis dropped from the JSON export — constant for the whole file and useless inside it.findings_exportedwith the format and the count.Frontend
The Bulk Export control is now visible whenever there are findings, and its label states the scope: Export Selected (N) when something is checked, Export All (N) when nothing is. SARIF joins CSV and JSON in the dropdown. The button disables while an export is in flight, and a failure raises an error toast instead of downloading an empty file.
Removed
serializeFindingsToCSV,escapeCSV,exportFindingsAsCSV, andexportFindingsAsJSONare gone — the endpoint replaced their only caller. Their column contract did not go with them; it is now asserted intesting/backend/unit/test_finding_export.py, including the comma/quote/newline escaping the old frontend test covered.downloadFileis kept as a generic helper (now a one-liner overdownloadBlob).The CSV keeps the same 14 columns in the same order, so existing scripts keep working. One deviation: line endings are now RFC 4180 CRLF, matching the task-report CSV, where the browser emitted bare LF.
Verification
testing/backend/integration/test_findings_export.py(new) — 28 teststesting/backend/unit/test_finding_export.py(new) — 22 testsvitest run— 64 files, 566 tests passedruff check backend testing/backend— cleantsc --noEmit— cleanquality-gate.cjs— 13 passed, 0 failed (its 1 warning is a pre-existing 2000ms animation, unrelated)Mutation-checked. Dropping owner scoping, treating
[]as "everything", removing redaction, leakingowner_id, skipping the cap, un-batching the count, and removing the de-duplication each fail a test.Two things I want to flag rather than bury:
test_duplicate_ids_do_not_duplicate_rowspasses with or without the explicit de-duplication, becauseIN (?, ?, ?)already collapses repeats. It is kept as a contract guard against a future rewrite that resolves ids one query at a time, and says so.ORDER BY ..., idtiebreaker survives mutation under SQLite, which happens to page tied rows consistently. It is there for PostgreSQL, which is under no such obligation. Also noted in the code.Overlap with #2394 — please read before merging either
@namann5's #2394 neutralizes CSV formula injection (CWE-1236) in
escapeCSVand inReportGenerator._sanitize_csv_cell. This PR deletesescapeCSVand moves findings-CSV generation to the backend, so merging the two naively would have quietly reopened that hole for the findings export.I checked rather than assumed, and the new writer was vulnerable:
So
finding_export.sanitize_csv_cellis now applied to every cell, deliberately mirroring #2394's semantics (=,+,-,@→ single-quote prefix) so the report CSV and the findings CSV cannot disagree about what is safe to hand a spreadsheet. Covered by unit tests and an end-to-end test that seeds a hostile finding title and asserts what lands in the file, and mutation-checked three ways (guard disabled, narrowed to=only, not wired into the row builder).Once both land, the two helpers should be folded into one. I left them separate only because #2394 is unmerged and this PR has to be safe standing alone. Happy to do that consolidation as a follow-up, or to rebase onto #2394 and use their helper directly if you would rather merge that one first.
Note on merge order
Conflicts, both verified with
git merge-tree:frontend/src/pages/Findings.tsxfrontend/src/utils/exportUtils.ts, its testHappy to rebase onto whichever you take first — just say which.